-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DOCS-908: Document part 1 of the cloud app API and move robot API #1706
Conversation
Co-authored-by: Sierra Guequierre <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some clarification/speaking to user
weight: 10 | ||
tags: ["robot state", "services"] | ||
# SME: Cheuk | ||
--- | ||
|
||
Robot Service constitutes a minimal set of APIs that most robots (Viam Server, [Viam Python SDK](https://python.viam.dev/) and various SDKs) should support. | ||
Users will likely use the Robot Service as an entrypoint to interact with Viam robots and provide a way to get updates from the robot as a whole. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... session management api, robot api, vision api, geometries/spatial integrations apis |
(opt) Could link with the shortcodes here to these methods tables. excited to see how this page turns out, agree it'd be a great value add!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will leave for a different PR/ticket
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed with Sierra's comments, and leaving a few little language flow comments. Otherwise LGTM!
docs/program/apis/cloud.md
Outdated
|
||
## API | ||
|
||
{{% alert title="Tip" color="tip" %}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if this is required for every usage of the Cloud API, I would recommend making this a prerequisite step of some kind, rather than a skippable Tip.
The copy "To use ..." is perfect though, that's an ideal intro.
The outro text "Then, run methods on..." could then be something like:
Once you have instantiated an
AppClient
, you can run Cloud API methods against thecloud
object according to the code examples for each method below.
Or similar. Not sure how to introduce cloud
here, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Maybe I should make it a section analogous to the "Control your ___ with..." section on component pages like https://docs.viam.com/components/base/#control-your-base-with-viams-client-sdk-libraries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! That looks like just what we need here!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd done it as a tip initially because it is different so didn't want it to look as though it wasn't different, but I think it'll work! will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh also I may have looked and seen this as a tip: https://docs.viam.com/services/vision/#api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh I could see that tip having a similar problem there as well: seemingly-skippable, yet required! Anyways, definitely out of scope for this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, and on many other service pages as well! Though personally I think the tips are so colorful and eye-catching that it might be ok esp if we changed them to important or something. But indeed a problem for another PR!
Co-authored-by: Sierra Guequierre <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some methods comments that I missed before
Co-authored-by: Sierra Guequierre <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarifying comments for GRPC error raised suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per offline convo, I only reviewed the code samples in docs/program/apis/cloud.md
. A couple small comments but generally looks good! I would suggest trying all of these code samples briefly before merging just to make sure all the syntax is correct.
docs/program/apis/cloud.md
Outdated
- The second (`[1]`) is a list of organization invites. | ||
|
||
```python {class="line-numbers linkable-line-numbers"} | ||
member_list = await cloud.list_organization_members() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest breaking up the return here to (member_list, invite_list)
or something like that.
Co-authored-by: Ethan <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! did you get a chance to test the code samples at all?
Ethan yeah, good call, I just finished testing them all. Updated connect code to have all the things including closing the connection. Found a Python docs bug and made a PR accordingly over there. |
@npentrel Sorry at first I'd thought you meant on the draft page but got it. Just added no_list true so should be good to go? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM % two more comments
Co-authored-by: Naomi Pentrel <[email protected]>
You can view a rendered version of the docs from this PR at https://docs-test.viam.dev/414ac5dcdcad22ffd75d5f89ba0be7d86bf9d6ad/public |
No description provided.